Implement aws.ecs.* resource attributes #2626

mmanciop · 2022-08-06T20:46:36Z

Implemented retrieving the aws.ecs.* resource attributes in go.opentelemetry.io/detectors/aws/ecs based on the ECS Metadata v4 endpoint.

detectors/aws/ecs/ecs_test.go

codecov · 2022-08-07T15:00:00Z

Codecov Report

Merging #2626 (f3c4f53) into main (15ceaa2) will decrease coverage by 0.7%.
The diff coverage is 0.0%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2626     +/-   ##
=======================================
- Coverage   69.7%   68.9%   -0.8%     
=======================================
  Files        147     147             
  Lines       6806    6875     +69     
=======================================
- Hits        4746    4743      -3     
- Misses      1944    2014     +70     
- Partials     116     118      +2

Impacted Files	Coverage Δ
detectors/aws/ecs/ecs.go	`18.0% <0.0%> (-36.8%)`	⬇️

mmanciop · 2022-08-12T15:30:54Z

There's discussion on open-telemetry/opentelemetry-java#4574 (comment) whether the ARNs like the one this PR builds are actually correct. Until we are use, let's not merge :D

mmanciop · 2022-08-12T16:03:58Z

Update: The uncertainty about ARNs seems sorted out

bryan-aguilar · 2022-08-15T18:48:22Z

Can we add test cases that utilize the fargate examples responses also?
Edit: If these are the same we can disregard. I am still digging into the differences if any, and whether or not these additional test cases would help.

bryan-aguilar · 2022-08-16T03:23:15Z

Looks like there may be slight differences between fargate and ec2 metadata response. As a sanity check can we add those example responses linked in the comment above to the unit tests?

mmanciop · 2022-08-16T14:24:46Z

Looks like there may be slight differences between fargate and ec2 metadata response. As a sanity check can we add those example responses linked in the comment above to the unit tests?

Done in 2ef2026. There are no factual differences that this patch cares about.

bryan-aguilar

From the ADOT team perspective this looks good to go.
Any long term concerns with using AWS ECS Metadata Go as it describes itself being built for fargate services?

CHANGELOG.md

Aneurysm9 · 2022-08-17T03:56:14Z

detectors/aws/ecs/ecs.go

+		semconv.AWSLogGroupNamesKey.String(logsOptions.AwsLogsGroup),
+		semconv.AWSLogGroupARNsKey.String(fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:*", logsRegion, awsAccount, logsOptions.AwsLogsGroup)),
+		semconv.AWSLogStreamNamesKey.String(logsOptions.AwsLogsStream),
+		semconv.AWSLogStreamARNsKey.String(fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:log-stream:%s", logsRegion, awsAccount, logsOptions.AwsLogsGroup, logsOptions.AwsLogsStream)),


Suggested change

semconv.AWSLogGroupNamesKey.String(logsOptions.AwsLogsGroup),

semconv.AWSLogGroupARNsKey.String(fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:*", logsRegion, awsAccount, logsOptions.AwsLogsGroup)),

semconv.AWSLogStreamNamesKey.String(logsOptions.AwsLogsStream),

semconv.AWSLogStreamARNsKey.String(fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:log-stream:%s", logsRegion, awsAccount, logsOptions.AwsLogsGroup, logsOptions.AwsLogsStream)),

semconv.AWSLogGroupARNsKey.StringSlice([]string{fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:*", logsRegion, awsAccount, logsOptions.AwsLogsGroup)}),

semconv.AWSLogStreamARNsKey.StringSlice([]string{fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:log-stream:%s", logsRegion, awsAccount, logsOptions.AwsLogsGroup, logsOptions.AwsLogsStream)}),

These are duplicative, so I'd suggest only including the ARN as the name can be extracted from it.

The values are expected to be string[].

String slices introduced in 4e857dd. I am not sold on skipping attributes though, see #2626 (comment).

detectors/aws/ecs/ecs.go

detectors/aws/ecs/ecs_test.go

mmanciop · 2022-08-17T10:58:59Z

@Aneurysm9 about the duplication of Log Group|Stream Name|ARN attributes, I implemented the spec: https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/cloud_provider/aws/logs/ . They may be duplicative (provided that the backend behind does processing, which is not the case with some OSS backends that have only limited query capabilities), but at the end of the day, if we wanna drop them, I'd rather first they get dropped from the spec.

Aneurysm9 · 2022-08-17T19:12:17Z

All of the log group/stream fields are optional. There is no requirement to include all of them if any of them are included.

mmanciop · 2022-08-18T07:04:55Z

All of the log group/stream fields are optional. There is no requirement to include all of them if any of them are included.

IMO, this would wreak havoc with less sophisticated tracing backends that do not have processing capabilities to normalize the attributes, as the same Log stream or group could be represented with different values.

Aneurysm9 · 2022-10-07T16:57:16Z

@mmanciop This requires an additional approval from @open-telemetry/go-approvers before it can be merged.

dmathieu · 2022-10-11T09:46:36Z

detectors/aws/ecs/ecs.go

 	"strings"

+	ecsmetadata "github.com/brunoscheufler/aws-ecs-metadata-go"


This repository has 3 stars and 14 commits. I wonder how reliable it can be in the future.
It's also fairly simple. So we can also extract its content and bring it back into this package later on if necessary.

I was committing fixed to that repo. The maintainer was nice and responsive. If worse comes to worse, can always be incorporated here.

mmanciop · 2022-10-13T14:38:55Z

@Aneurysm9 does this ring a bell?

panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: The requested service provider could not be loaded or initialized. [recovered]
panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: The requested service provider could not be loaded or initialized.

I copied the test code from the Ec2Detector and the tests pass on my Mac; am I perhaps missing something in the CI configs?

(The fact that the CI does not trigger every time I push a commit makes it is even more daunting to troubleshoot...)

…ctors/aws/ecs

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>

After a surreal session of debugging, it turns out that httptest.NewServer fails on Windows by failing to bind the server socket unless the GoLang package name contains "test". To deal with that, I split the tests between "integration tests" needing a HTTP server into "ecs/test", and the other in "ecs". In order to work around the need of being resilient to the lack of /proc/self/cgroup (which happens in our tests but, above all, when running containers on Windows), the ECVS detector is now more lenient to not finding the container id.

mmanciop requested review from jmacd, MrAlias, Aneurysm9, evantorrie, XSAM, dashpole, MadVikingGod, pellared, hanyuancheung and dmathieu as code owners August 6, 2022 20:46

hanyuancheung reviewed Aug 7, 2022

View reviewed changes

detectors/aws/ecs/ecs_test.go Outdated Show resolved Hide resolved

detectors/aws/ecs/ecs_test.go Outdated Show resolved Hide resolved

mmanciop marked this pull request as draft August 12, 2022 15:30

mmanciop marked this pull request as ready for review August 12, 2022 16:00

bryan-aguilar approved these changes Aug 16, 2022

View reviewed changes

Kausik-A mentioned this pull request Aug 16, 2022

Implement aws.ecs.* resource attributes open-telemetry/opentelemetry-python-contrib#1212

Merged

7 tasks

Aneurysm9 reviewed Aug 17, 2022

View reviewed changes

mmanciop force-pushed the aws_ecs_resource_attributes branch from 5804768 to 4e857dd Compare August 17, 2022 10:56

mmanciop force-pushed the aws_ecs_resource_attributes branch from 838e75c to 6fb0790 Compare September 8, 2022 07:54

Aneurysm9 approved these changes Sep 13, 2022

View reviewed changes

mmanciop force-pushed the aws_ecs_resource_attributes branch 2 times, most recently from a47260b to f379b94 Compare September 17, 2022 05:45

mmanciop force-pushed the aws_ecs_resource_attributes branch 2 times, most recently from 2fe086b to 0654cc1 Compare September 18, 2022 07:05

mmanciop requested a review from hanyuancheung September 28, 2022 16:20

mmanciop changed the title ~~Implemented aws.ecs.* resource attributes~~ Implement aws.ecs.* resource attributes Sep 29, 2022

dmathieu approved these changes Oct 11, 2022

View reviewed changes

mmanciop requested review from bryan-aguilar and removed request for hanyuancheung October 13, 2022 07:48

Michele Mancioppi and others added 6 commits October 23, 2022 14:15

Implemented aws.ecs.* resource attributes in go.opentelemetry.io/dete…

9d8be4e

…ctors/aws/ecs

Update detectors/aws/ecs/ecs_test.go

6a1b274

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>

Update detectors/aws/ecs/ecs_test.go

8b5d348

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>

fix: lower-case the value of aws.ecs.launchtype

a5bde89

Add aws.logs.* support, remove spurious /aws prefix from log group

28bc900

Add tests for V4 on Fargate launch type

e518c40

mmanciop force-pushed the aws_ecs_resource_attributes branch from abbc6a2 to e518c40 Compare October 23, 2022 12:47

Michele Mancioppi added 2 commits October 23, 2022 14:49

Rebase

400645f

mmanciop requested review from hanyuancheung and bryan-aguilar and removed request for bryan-aguilar and hanyuancheung October 24, 2022 15:33

Merge branch 'main' into aws_ecs_resource_attributes

45bc0f8

mmanciop mentioned this pull request Oct 29, 2022

feat: implement cloud{provider,platform}, aws.{ecs.*,log.*} in ECS detector open-telemetry/opentelemetry-php-contrib#83

Merged

MrAlias added the detector: aws:ecs label Nov 2, 2022

Michele Mancioppi added 2 commits November 18, 2022 18:04

Merge branch 'main' into aws_ecs_resource_attributes

db4e0a8

Merge branch 'main' into aws_ecs_resource_attributes

f3c4f53

Aneurysm9 merged commit 01b39e7 into open-telemetry:main Nov 23, 2022

MadVikingGod mentioned this pull request Dec 5, 2022

Release v1.12.0/0.37.0/0.6.0 #3067

Merged

pellared added this to the untracked milestone Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement aws.ecs.* resource attributes #2626

Implement aws.ecs.* resource attributes #2626

mmanciop commented Aug 6, 2022

codecov bot commented Aug 7, 2022 •

edited

Loading

mmanciop commented Aug 12, 2022 •

edited

Loading

mmanciop commented Aug 12, 2022

bryan-aguilar commented Aug 15, 2022 •

edited

Loading

bryan-aguilar commented Aug 16, 2022

mmanciop commented Aug 16, 2022

bryan-aguilar left a comment

Aneurysm9 Aug 17, 2022

mmanciop Aug 18, 2022

mmanciop commented Aug 17, 2022

Aneurysm9 commented Aug 17, 2022

mmanciop commented Aug 18, 2022

Aneurysm9 commented Oct 7, 2022

dmathieu Oct 11, 2022

mmanciop Oct 13, 2022

mmanciop commented Oct 13, 2022 •

edited

Loading

		"strings"

		ecsmetadata "github.com/brunoscheufler/aws-ecs-metadata-go"

Implement aws.ecs.* resource attributes #2626

Implement aws.ecs.* resource attributes #2626

Conversation

mmanciop commented Aug 6, 2022

codecov bot commented Aug 7, 2022 • edited Loading

Codecov Report

mmanciop commented Aug 12, 2022 • edited Loading

mmanciop commented Aug 12, 2022

bryan-aguilar commented Aug 15, 2022 • edited Loading

bryan-aguilar commented Aug 16, 2022

mmanciop commented Aug 16, 2022

bryan-aguilar left a comment

Choose a reason for hiding this comment

Aneurysm9 Aug 17, 2022

Choose a reason for hiding this comment

mmanciop Aug 18, 2022

Choose a reason for hiding this comment

mmanciop commented Aug 17, 2022

Aneurysm9 commented Aug 17, 2022

mmanciop commented Aug 18, 2022

Aneurysm9 commented Oct 7, 2022

dmathieu Oct 11, 2022

Choose a reason for hiding this comment

mmanciop Oct 13, 2022

Choose a reason for hiding this comment

mmanciop commented Oct 13, 2022 • edited Loading

codecov bot commented Aug 7, 2022 •

edited

Loading

mmanciop commented Aug 12, 2022 •

edited

Loading

bryan-aguilar commented Aug 15, 2022 •

edited

Loading

mmanciop commented Oct 13, 2022 •

edited

Loading